fix: use correct rollback tails for empty frames without history #158
Conversation
src/witness/oracle.rs
Outdated
| @@ -552,8 +552,35 @@ pub fn create_artifacts_from_tracer< | |||
| // wherever we have this marker we should look at the tail of the item right before it | |||
| let pos = log_position_mapping[&rollback_tail_marker]; | |||
| let tail = if pos == -1 { | |||
| // empty | |||
| global_end_of_storage_log | |||
| if frame_index > 1 { | |||
There was a problem hiding this comment.
FYI - this is more than 'adding tests' - please update the PR description
There was a problem hiding this comment.
@mm-zk you are right. Even more, proposed changes do not fix corresponding issue. I will rewrite this part of logic
src/witness/oracle.rs
Outdated
| // instead we should use previous frame's data | ||
| let pos_previous_frame_forward_tail = log_position_mapping | ||
| [&ExtendedLogQuery::FrameForwardTailMarker(frame_index - 1)]; | ||
| if pos_previous_frame_forward_tail != -1 { |
There was a problem hiding this comment.
please add more explanations, as I couldn't fully understand the code.
There was a problem hiding this comment.
Idea: since we do not have rollback tail marker, we should try to use forward tail of the parent of this frame.
Incorrect assumption: if parent is empty too, than it is "root" frame.
Actually this is wrong. We can have any hierarchy of empty frames
There was a problem hiding this comment.
Changed the code and added more comments
src/witness/oracle.rs
Outdated
|
|
||
| if pos_forward_head == -1 && pos_forward_tail == -1 && pos_rollback_head == -1 { | ||
| // absolutely empty frame, most likely leaf one, | ||
| // but we can not just use global end, |
There was a problem hiding this comment.
why we cannot use global end?
There was a problem hiding this comment.
Because we can have some state changes after this rollback. But our rollback tail should be equal to the last state change tail before the rollback.
Global end was used under assumption that situation without rollback marker can happen only in trivial cases (like entry bytecode without any state changes)
src/witness/oracle.rs
Outdated
| if frame_index > 1 { | ||
| // empty frame | ||
| let pos_forward_head = | ||
| log_position_mapping[&ExtendedLogQuery::FrameForwardHeadMarker(frame_index)]; |
There was a problem hiding this comment.
do we need similar code in zksync-era's oracle?
There was a problem hiding this comment.
do we need similar code in zksync-era's oracle?
I'm not sure that I understand your question: zksync-era calls this code via external_calls.rs
|
Added some todos. I'll deal with them in the next pull requests |
0xVolosnikov
changed the title
|
Fixed in #165 |
What ❔
At the moment, witness generation for empty frames that do not have any previous history of interactions with the state is implemented incorrectly. In such a situation, the wrong log may be selected as the corresponding tail of the rollback queue. The problem is exotic and occurs only in artificial tests.
Why ❔
Checklist
zk fmtandzk lint.